Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(transport-http): layer client #1227

Merged
merged 33 commits into from
Sep 18, 2024
Merged

feat(transport-http): layer client #1227

merged 33 commits into from
Sep 18, 2024

Conversation

yash-atreya
Copy link
Member

@yash-atreya yash-atreya commented Sep 2, 2024 •
edited
Loading

Motivation

Currently, there is no way to modify the absolute low-level request such as http::Request.

This is due to the limitation that transport layers work over alloy_json_rpc::RequestPacket and not http::Request.

Solution

Making the hyper transport HTTP client generic over an underlying service.

By default, it serves as a vanilla hyper client like before (use HyperTransport::new()), with the added benefit of adding tower layers on top of the hyper::Client and using that in HyperTransport::with_service(service).

This new set of layers will work over http::Request which is used by hyper allowing us to modify request properties such as headers.

A HyperTransport with layers can be initialized like so:

let hyper_client = Client::builder(TokioExecutor::new()).build_http::<Full<HyperBytes>>();

let service = tower::ServiceBuilder::new().layer(SetRequestHeaderLayer::if_not_present(header::USER_AGENT, HeaderValue::from_static("alloy app"))).service(hyper_client);

let layer_transport = alloy_transport_http::HyperTransport::with_service(service);

let rpc_client = alloy_rpc_client::RpcClient::new(layer_transport, true);

This will help implement a JWT auth layer #1161 that refreshes auth tokens.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@yash-atreya
Copy link
Member Author

The LoggingLayer is just for testing and demo, I'll be removing it

mattsse
mattsse reviewed Sep 2, 2024
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, this should work.

I'd like to see some more advanced tests/exampls here that make use of some tower-http components

for example auth

see also https://github.com/tower-rs/tower-http/blob/3f98dc19bc6c70aa6e4912b63538d89443f03b19/examples/axum-key-value-store/src/main.rs

crates/transport-http/src/layers/logging.rs Outdated Show resolved Hide resolved
@mattsse mattsse mentioned this pull request Sep 4, 2024
Closed
3 tasks
@yash-atreya yash-atreya changed the title [wip] feat(transport-http): reqwest layer client [wip] feat(transport-http): layer client Sep 7, 2024
@yash-atreya yash-atreya marked this pull request as ready for review September 9, 2024 10:46
@yash-atreya
Copy link
Member Author

What we currently have working is the following:

  • A new HTTP transport client based on hyper that lets users create and layer services on top of each other that modify the underlying http::Request or http::Response types.
  • This is enabled through the use of tower::ServiceBuilder
  • One can now write a layer with a service that works over http::Request. Examples include [Feature] Add JWT layer for transport #1161 that modifies the auth header, and/or append a X-Flashbots-Signature https://docs.flashbots.net/flashbots-auction/advanced/rpc-endpoint#authentication
  • tower-http layers that do not change the response type or error type work as well, this is due to the restrictive trait bounds we've used.
  • All of this is very useful but not ideal, we want to work with all tower-http layers

TODO

  • tower-http layers like TimeoutLayer or TraceLayer don't work because they modify the response type to something like http::Response<TimeoutBody<Incoming>> which is incompatible with expected response type i.e type HyperResponse = http::Response<Incoming> type. Thinking of ways to make this more flexible.

cc @mattsse @klkvr

@yash-atreya yash-atreya changed the title [wip] feat(transport-http): layer client feat(transport-http): layer client Sep 10, 2024
mattsse
mattsse requested changes Sep 11, 2024
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits

overall this looks good

crates/transport-http/Cargo.toml Outdated Show resolved Hide resolved
crates/transport-http/src/hyper_layer_transport.rs Outdated Show resolved Hide resolved
crates/transport-http/src/hyper_layer_transport.rs Outdated Show resolved Hide resolved
@klkvr
Copy link
Member

klkvr commented Sep 11, 2024 •
edited
Loading

  • tower-http layers like TimeoutLayer or TraceLayer don't work because they modify the response type to something like http::Response<TimeoutBody<Incoming>> which is incompatible with expected response type i.e type HyperResponse = http::Response<Incoming> type. Thinking of ways to make this more flexible.

I think we could just use Body trait to restrict response body? https://docs.rs/http-body/1.0.0/http_body/trait.Body.html

something like Response<T> where T: Body<Data = Bytes>

klkvr
klkvr requested changes Sep 17, 2024
View reviewed changes
crates/transport-http/src/hyper_transport.rs Outdated Show resolved Hide resolved
crates/transport-http/src/hyper_transport.rs Outdated Show resolved Hide resolved
crates/transport-http/src/lib.rs Outdated Show resolved Hide resolved
@yash-atreya
Copy link
Member Author

I am not sure we should have that. I believe that point of TransportConnect is in knowing how to instantiate transport. If we require user to provide it with a ready transport instance, then user can just directly configure this instance instead of relying on TransportConnect

we can only have TransportConnect implemented for default hyper transport I think

This makes sense!
Removed the transport from HttpConnect, and impl TransportConnect for default HyperTransport

klkvr
klkvr requested changes Sep 18, 2024
View reviewed changes
Copy link
Member

@klkvr klkvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits

crates/transport-http/src/hyper_transport.rs Outdated Show resolved Hide resolved
crates/transport-http/src/hyper_transport.rs Outdated Show resolved Hide resolved
klkvr
klkvr approved these changes Sep 18, 2024
View reviewed changes
Copy link
Member

@klkvr klkvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mattsse mattsse merged commit 68115f5 into main Sep 18, 2024
26 checks passed
@mattsse mattsse deleted the yash/reqwest-tower-integration branch September 18, 2024 09:43
@yash-atreya yash-atreya mentioned this pull request Sep 19, 2024
Merged
3 tasks
lwedge99 pushed a commit to sentioxyz/alloy that referenced this pull request Oct 8, 2024
@yash-atreya @lwedge99
feat(transport-http): layer client (alloy-rs#1227)
1acd39c 
* feat(transport): bare minimal reqwest-tower integration

* feat(transport-http): reqwest-tower layer client

* feat(transport-http): LayerClient

* fix: feature gate layer transport

* rm logging layer

* feat(transport-http): hyper layer transport

* hyper layer transport test

* test with tower-http layers

* rm reqwest layer transport

* rm trait bounds for new

* nit

* unify hyper transports

* rm TransportConnect for HyperLayerTransport

* make request generic

* unify hyper transports

* nit

* nit

* nit

* rm unintended reqwest default

* rename HyperLayerTransport to HyperTransport

* rename file

* nit

* fix: rm transport from HttpConnect

* fix: rm url from HyperTransport, infer it from Http

* clippy

* fix

* impl Http<Hyper>

* fix

* rename

* nit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@klkvr klkvr klkvr approved these changes

@mattsse mattsse mattsse approved these changes

@DaniPopes DaniPopes Awaiting requested review from DaniPopes DaniPopes is a code owner

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

@onbjerg onbjerg Awaiting requested review from onbjerg onbjerg is a code owner

@prestwich prestwich Awaiting requested review from prestwich

@emhane emhane Awaiting requested review from emhane emhane is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yash-atreya @klkvr @mattsse